-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revision of CMS inclusive DY data #2238
Conversation
nnpdf_data/nnpdf_data/commondata/CMS_WPWM_8TEV_MUON/uncertainties.yaml
Outdated
Show resolved
Hide resolved
nnpdf_data/nnpdf_data/commondata/CMS_Z0_7TEV_DIMUON/uncertainties.yaml
Outdated
Show resolved
Hide resolved
And this is the report after these changes: report. Nothing has changed compared to the previous versions. I think this one is ready for merging. Let's wait for the tests. |
fec31c7
to
a1d5a5a
Compare
@scarlehoff I don't understand that error message that I get from the test
Can you help me? |
It means that you need to use the label again. Basically it means that the bot would get results different than what you commited (@Radonirinaunimi and I were unable to make the -1 signs agree between computers so we decided to take the bot as the ultimate truth) |
How is that possible 🫤? |
I think it is the eigen value decomposition of the covmat in either scipy or numpy that is basically non reproducible (you would need to take the values and post process them), but having the bot generating all data was something we wanted to do anyway. |
Ok, I think now this is solved. Wait for the tests, and then I think it can be merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Beside the comment about the kinematics_override
this is good to go.
The only other thing that might be interesting to change is the Z0
variable from mass squared to just mass, so that the plots have the mass in the title
(right now the value is of the mass due to the kinematics override, but the units are of course mass squared)
Up to you if you want to change it, the information is of course the same, I just feel the non-squared mass is easier to read.
@@ -31,13 +31,13 @@ implemented_observables: | |||
dataset_label: "CMS Drell-Yan 2D 7 TeV 2011" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR addresses the four datasets listed below:
These datasets were already re-implemented in the new format by TR. I think the things to do here are:
process_type
with the processes inprocess_options
.See the reports for these datasets before I apply any change: report